[fix](stream) Mark streams stale when base tables are dropped - #66287
[fix](stream) Mark streams stale when base tables are dropped#66287seawinde wants to merge 8 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
TPC-H: Total hot run time: 29693 ms |
TPC-DS: Total hot run time: 177743 ms |
ClickBench: Total hot run time: 24.87 s |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes: the drop/recovery behavior is largely implemented, but two correctness issues remain: concurrent readers can null-dereference the volatile cached base table, and available streams report stale qualifiers after supported same-ID rename operations.
Review checkpoints
- Goal and proof: derived disabled/stale state fixes ordinary recoverable drop, original-ID recovery, force drop, and same-name/new-ID recreation. The added unit and IVM regression coverage exercise those paths, but rename coverage would expose MAIN-001.
- Scope and clarity: the change is focused on stream lifecycle state and metadata; no unrelated source changes were found.
- Concurrency: drop/recovery publication and all getter callers were traced. MAIN-002 is a reachable shared-reader race; no separate persistent cleanup/planner race was substantiated.
- Lifecycle: the runtime cache,
Table.isDropped, database registration, recycle-bin recovery, checkpoint load, and journal replay were reviewed. ID-based recovery is sound apart from the accepted issues. - Configuration and compatibility: no configuration, storage-format, function-symbol, schema-column, or FE-BE protocol change is introduced.
- Parallel paths and conditions: table drop, force drop, database drop/recovery, table/database rename, renamed recovery, planner binding, cleanup, metadata, and
SHOW CREATE STREAMwere checked. Rename paths produce MAIN-001. - Tests and results: test SQL ordering/output conventions are correct and the new result file is marked generated. This runner is review-only, so I did not execute builds or tests.
- Observability: the missing-table state is visible through
table_streamsand planner errors; lowering repeated missing-resolution logging to debug is acceptable. - Persistence, transactions, and data writes: explicit flags and base-table IDs remain serialized; no new transaction, EditLog schema, data-write, or master-failover issue was found.
- Performance and other concerns: repeated missing-table resolution is bounded to metadata/cleanup lookups; no additional material performance, memory, or static-initialization issue was found.
- User focus: no additional user-provided review focus was supplied; the full PR was reviewed.
Review completion: complete for the frozen head after the required bounded subagent convergence and final live-state verification.
|
|
||
| public TableIf getBaseTableNullable() { | ||
| if (baseTable instanceof Table && ((Table) baseTable).isDropped) { | ||
| baseTable = null; |
There was a problem hiding this comment.
[P1] Snapshot the volatile base table before dereferencing it
The two baseTable occurrences in this condition are separate volatile reads. After a drop marks the cached table dropped, two concurrent readers can both pass the instanceof; one can then execute baseTable = null before the other evaluates ((Table) baseTable).isDropped, causing that reader to dereference null. Concurrent table_streams requests are sufficient because the stream read lock admits multiple readers. Copy the field to one local snapshot, inspect that snapshot, and resolve/publish the replacement once.
There was a problem hiding this comment.
Fixed in 6baebd9e78a. getBaseTableNullable() now snapshots the volatile field into cachedBaseTable and uses that local for the dropped check and return path. Clearing the shared cache can no longer change the value being dereferenced by another reader.
| // STREAM_COMMENT | ||
| trow.addToColumnValue(new TCell().setStringVal(stream.getComment())); | ||
| List<String> baseTableQualifiers = stream.getBaseTableFullQualifiers(); | ||
| // BASE_TABLE_NAME |
There was a problem hiding this comment.
[P1] Use live qualifiers while the base table is available
TableStreamBaseTableInfo keeps the creation-time names but resolves internal tables by ID. After a supported same-ID rename (ALTER TABLE old RENAME new or RECOVER TABLE old AS new), this line still emits old even though the stream resolves successfully and is reported enabled; SHOW CREATE STREAM emits new, and callers filtering table_streams by the current base name miss the stream. Resolve baseTable first and use baseTable.getFullQualifiers() when it is non-null, falling back to the stored qualifiers only for the unavailable-table case.
There was a problem hiding this comment.
Fixed in 6baebd9e78a. table_streams now resolves the base table first and uses baseTable.getFullQualifiers() while it is available. Stored qualifiers are only the final fallback when neither the live table nor a same-ID recycle-bin entry is available. The unit test covers ALTER TABLE ... RENAME and RECOVER TABLE ... AS.
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
2d14096 to
bf0a7de
Compare
|
run buildall |
TPC-H: Total hot run time: 28939 ms |
TPC-DS: Total hot run time: 178423 ms |
bf0a7de to
8a8eec2
Compare
|
run buildall |
ClickBench: Total hot run time: 24.73 s |
|
/review |
TPC-H: Total hot run time: 29310 ms |
TPC-DS: Total hot run time: 177733 ms |
ClickBench: Total hot run time: 24.69 s |
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Aug 5th, 2026 4:32 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
FE Regression Coverage ReportIncrement line coverage |
|
/review |
There was a problem hiding this comment.
Request changes: the sequential drop/recovery path works, but the current head still has the two existing P1 blockers and two additional metadata/DDL consistency issues: one table_streams row can mix availability snapshots during concurrent DDL, and SHOW CREATE STREAM loses the latest same-ID base identity while the table is recoverably absent.
Review checkpoints
- Goal and proof: ordinary drop, same-name recovery, force drop, and same-name/new-ID recreation derive the intended state. The added unit/regression tests and green FE/P0 CI prove those sequential paths, but not the accepted concurrent-row or
SHOW CREATEcases. - Scope and clarity: the source change is focused on stream availability, metadata, logging, and tests.
- Concurrency: the stream cache, database/table publication, metadata readers, planner binding, and cleanup locks were traced. Existing thread
r3682055286covers the shared-reader NPE;MAIN-001covers the distinct single-row snapshot race. No deadlock or further lock-order issue was found. - Lifecycle: dropped-cache invalidation, force drop, original-ID recovery, rename/recover-as and subsequent re-drop, image load, and replay were reviewed.
MAIN-002is the newly exposed unavailable-state DDL regression; the remaining ID-based reattachment is sound. - Configuration and compatibility: no new config, storage format, function symbol, schema column, or FE-BE protocol is introduced; no rolling-upgrade incompatibility was found.
- Parallel paths and conditions: metadata,
SHOW CREATE, Nereids collection/binding, cleanup, leader/replay, force-drop, and rename paths were checked. Existing threadr3682055289covers the live-qualifier path; the pre-existing Nereids creation-name lookup was not charged to this diff. - Tests and results: ordering/output conventions are correct. Current CI reports FE UT, P0 regression, checkstyle, compile, and the other completed checks green. This review runner forbids local builds/tests, so none were run here; latch-controlled concurrency plus unavailable
SHOW CREATEcoverage for simple drop and rename/recover-as followed by re-drop are still needed. - Observability: lowering expected missing-table resolution to debug and exposing stale state are appropriate; no additional metric is needed.
- Persistence, transactions, and data writes: IDs and explicit flags remain serialized, transient availability is correctly derived, and no EditLog schema, data-write atomicity, master-failover, or transaction-path issue was found.
- Performance and remaining concerns: repeated missing-table lookups are limited to metadata/cleanup paths; no material CPU, memory, static-initialization, or FE-BE propagation issue was found beyond the reported findings.
- User focus: no additional user-provided focus was supplied; the whole PR was reviewed.
Review completion: complete for the frozen head after required bounded subagent convergence and final live-state verification.
| // BASE_TABLE_TYPE | ||
| trow.addToColumnValue(new TCell().setStringVal(baseTable.getType().name())); | ||
| } | ||
| trow.addToColumnValue(new TCell().setStringVal( |
There was a problem hiding this comment.
[P2] Derive the row from one availability snapshot
baseTable is reused only for BASE_TABLE_TYPE; isDisabled(), isStale(), and getStaleReason() each resolve it again. Because base-table drop/recovery uses different locks from this stream read lock, a recovery after the type lookup can emit N/A with enabled/non-stale state, while a drop can emit OLAP with disabled/stale state. Resolve availability once per row and derive all four fields from that snapshot while still combining the persisted flags; a latch-controlled DDL test would cover the interleaving.
There was a problem hiding this comment.
Fixed in 6baebd9e78a. fillTableStreamValuesMetadataResult() resolves baseTable once per TRow and passes that snapshot to the qualifier, disabled, stale, and stale-reason calculations. The overloads still combine the snapshot with the persisted disabled/stale flags, so one row can no longer mix pre-drop/post-drop or pre-recovery/post-recovery availability.
| } | ||
|
|
||
| public TableIf getBaseTableNullable() { | ||
| if (baseTable instanceof Table && ((Table) baseTable).isDropped) { |
There was a problem hiding this comment.
[P2] Preserve the latest base identity before clearing it
Once this branch clears the dropped table, getBaseTableNullable() returns null and Env.getDdlStmt() emits ON TABLE UNKNOWN, making SHOW CREATE STREAM non-round-trippable during a recoverable drop. Falling back directly to TableStreamBaseTableInfo is not enough after old is renamed/recovered as new and dropped again: that descriptor still says old, while the recycle bin can recover only new. Preserve the latest same-ID live qualifiers in replay-equivalent state before clearing them (or resolve that recycle entry by ID), and cover both a simple drop and rename/recover-as followed by re-drop.
There was a problem hiding this comment.
Fixed in 6baebd9e78a. For an unavailable internal base table, display qualifiers are resolved from the recycle bin by (dbId, tableId) before falling back to creation-time qualifiers. SHOW CREATE STREAM now uses those qualifiers instead of UNKNOWN. Coverage includes a simple recoverable drop, rename then drop, Gson deserialization as a replay/restart-equivalent path, RECOVER TABLE ... AS, and a second drop retaining the recovered name.
| trow.addToColumnValue(new TCell().setStringVal(stream.getScanTypeString())); | ||
| // STREAM_COMMENT | ||
| trow.addToColumnValue(new TCell().setStringVal(stream.getComment())); | ||
| List<String> baseTableQualifiers = stream.getBaseTableFullQualifiers(); |
There was a problem hiding this comment.
[P1] 基表同 ID 重命名后，table_streams 仍返回旧名称
getBaseTableFullQualifiers() 永远返回创建 stream 时保存的名称(基于basetable info)。
There was a problem hiding this comment.
Fixed in 6baebd9e78a. getBaseTableFullQualifiers() no longer always returns the creation-time qualifiers from baseTableInfo: when the same-ID base table is available, it returns the live table qualifiers, so table_streams reflects ALTER TABLE ... RENAME; when the table is recoverably dropped, it resolves the same-ID recycle-bin entry; only if neither can be resolved does it fall back to the creation-time qualifiers. table_streams also resolves the base table once per row and passes that snapshot into the qualifier calculation. Unit coverage includes ALTER TABLE ... RENAME and RECOVER TABLE ... AS.
|
run buildall |
|
/review |
### What problem does this PR solve? Issue Number: close apache#65418 Related PR: apache#66287 Problem Summary: The previous stable-ID locking fix cached a stream base table under its current name qualifier. A pre-lock rename could therefore overwrite an explicit relation already resolved under the same qualifier and make binding scan the wrong table. Keep the relation cache limited to SQL relations and expand each stream base table by stable ID only when constructing the existing ordered lock queue. ### Release note None ### Check List (For Author) - Test: FE unit test and FE build - ./run-fe-ut.sh --run org.apache.doris.nereids.StatementContextTest,org.apache.doris.nereids.trees.plans.ExplainTableStreamPlanTest - ./run-fe-ut.sh --run org.apache.doris.catalog.DropTableStreamTest - env DISABLE_BUILD_UI=ON ./build.sh --fe - Behavior changed: Yes. Explicit relation bindings are preserved while stream base tables are locked by stable ID. - Does this need documentation: No
a0238b2 to
a7850ad
Compare
|
run buildall |
|
/review |
### What problem does this PR solve? Issue Number: close apache#65418 Related PR: apache#66287 Problem Summary: Document why stream base tables are validated by stable ID without entering the qualifier relation cache, and why implicit base dependencies are expanded only in the local ID-ordered planner lock queue. The comments preserve the concurrency invariant behind the relation-cache collision fix. ### Release note None ### Check List (For Author) - Test: FE unit test - ./run-fe-ut.sh --run org.apache.doris.nereids.StatementContextTest,org.apache.doris.nereids.trees.plans.ExplainTableStreamPlanTest - Behavior changed: No - Does this need documentation: No
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
There was a problem hiding this comment.
Requesting changes: the sequential drop/recover behavior works, but three correctness and concurrency gaps remain. Recovery can expose a still-dropped table as available, MTMVs over streams lose the actual OLAP dependency and can remain stale, and a drop/recover race can bypass external-metadata preload and perform remote work under an internal table lock.
Critical checkpoint conclusions
- Goal and proof: the code makes streams disabled/stale while their stable-ID base is unavailable and restores them when that ID returns; the unit and regression additions prove the sequential path, but not the three inline scenarios.
- Scope: the 12-file FE/test change is focused on stream availability, qualifiers, planner dependencies, and coverage; no unrelated production change was found.
- Concurrency: metadata readers, recovery DDL, and planner threads access the volatile base cache under different database/table/recycle/stream locks. ID-ordered planner acquisition and release accounting are otherwise sound, but the publication and preload phase races remain.
- Lifecycle: normal/force/replay drop, recovery/replay recovery, rename, recycle erasure, Gson reload, and wrapper binding were traced. Stable IDs fence same-name replacements, and no lock-order cycle, static-initialization issue, or leaked lifecycle was found.
- Configuration: no configuration item was added or changed.
- Compatibility: no FE-BE symbol, thrift field, storage format, or new persisted field was introduced; existing image and replay state remains compatible.
- Parallel paths: query, insert-target, and MTMV maps all reach the ID-ordered lock expansion. Non-lock dependency consumers do not receive the implicit base, producing the MTMV issue called out inline; audit/view-dependency effects share that root cause.
- Conditions and error handling: missing bases fail planning and expected missing-base logging is appropriately DEBUG, but a cache-miss lookup does not apply the same dropped-state condition as the cached path.
- Test coverage: JUnit 5 and ordered regression coverage exercise sequential drop/recover, rename, force drop, reload, metadata, and lock-cache behavior. The accepted findings need an MTMV refresh case and latch-controlled recovery/preload races.
- Test results: the added
.outrows are ordered and consistent with the SQL. This review runner performed no build or test, as required by the review instructions, so author/CI results were not independently reproduced here. - Observability:
table_streamsexposes the new state and DEBUG logging avoids expected warning noise; no additional metric is required for this metadata path. - Transaction and persistence: no transaction protocol or EditLog schema changed; persisted stream identity and replay-equivalent paths were checked.
- Data writes: the production change does not add a data-write path; the MTMV finding affects refresh/freshness correctness after writes to the base.
- FE-BE variables and nullable handling: no new FE-BE variable or BE nullable path is involved.
- Performance: no general hot-path regression was found; the accepted preload race can move connector metadata I/O back under an internal table lock.
- Other issues: existing qualifier and relation-cache threads were duplicate-fenced, including the SHOW CREATE residual. No security-sensitive behavior was requested or reviewed, and no further non-duplicate issue remained.
User focus: review_focus.txt supplied no additional focus, so the entire PR and all required checkpoints were reviewed.
Review completion: converged after Round 2; both normal full-review subagents and the independent risk challenger returned NO_NEW_VALUABLE_FINDINGS, and every ledger candidate is accepted, dismissed, or duplicate-fenced.
| return null; | ||
| } | ||
| if (cachedBaseTable == null) { | ||
| cachedBaseTable = baseTableInfo.getTableNullable(); |
There was a problem hiding this comment.
[P2] Recheck dropped state after resolving a cache miss
During RECOVER TABLE, innerRecoverTable() holds A's write lock, but Database.registerTable() publishes A in the ID/name maps before calling unmarkDropped(). An unlocked table_streams request can therefore reach this cache-miss branch while A.isDropped is still true. Because the looked-up value is cached and returned without the dropped check applied to the initial snapshot, that row reports OLAP/enabled/non-stale before recovery has made the table available. Please make publication atomic to unlocked readers or apply the same dropped-state check to the resolved candidate, and add a latch test paused between map publication and unmarkDropped().
| // we need to collect stream table's base table as well | ||
| if (table instanceof BaseTableStream) { | ||
| collectFromTableStream((BaseTableStream) table, cascadesContext, tableFrom, unboundRelation); | ||
| collectFromTableStream((BaseTableStream) table); |
There was a problem hiding this comment.
[P1] Preserve the stream base in MTMV dependency tracking
Reduced plan:
MTMV M
UnboundRelation(S) // stream over OLAP table T
This change validates T but leaves StatementContext.tables containing only S; T is reintroduced only in the local lock queue. Both MTMV creation and every refresh build MTMVRelation from getTables().values(), so T is absent. Since S is not MTMVRelatedTableIf, isSyncWithBaseTable() treats it as synchronized and snapshot generation skips it, allowing a commit to T to leave M considered fresh and absent from T's invalidation relation. Keep the stable base in a separate ID/object-keyed dependency collection consumed by locking and MTMV relation/snapshot tracking (without putting it back in the qualifier cache), and add an async-MV-over-stream refresh test.
| } | ||
| if (tableIf instanceof BaseTableStream) { | ||
| // Mirror addTablesToLock(): a stream needs no plan lock itself, but its stable-ID base may need one. | ||
| TableIf baseTable = ((BaseTableStream) tableIf).getBaseTableNullable(); |
There was a problem hiding this comment.
[P2] Use one stream-base snapshot for preload and locking
For Join(S on internal A, external E), collection can validate A, A can be dropped before this preload gate (so the preload result is memoized as skipped), and A can recover before lock() resolves it again and acquires A's read lock. Binding E then still loads its snapshot/schema while that internal lock is held, defeating the preload boundary that keeps remote metadata work outside table locks. Freeze the stable stream-base dependency during collection and use the same set for both preload gating and ID-ordered locking (or conservatively treat a collected stream as a possible lock dependency), with a latch-controlled drop/gate/recover/lock test.
FE UT Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 28844 ms |
TPC-DS: Total hot run time: 158345 ms |
ClickBench: Total hot run time: 23.73 s |
FE Regression Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: close apache#65418 Related PR: apache#66287 Problem Summary: Stream base tables are implicit planning dependencies resolved by stable table ID. Keeping them in the qualifier-keyed relation cache can overwrite an explicitly resolved relation after concurrent renames, while resolving them again during preload or locking can select a different object snapshot. MTMV relation generation also omitted the underlying stream base after it was removed from the relation cache. Store exact implicit dependency objects separately, reuse them for preload and locking, include them in all-level MTMV dependencies, and reject dropped tables during cache-miss recovery resolution. ### Release note Streams continue to track and lock their original base tables by stable identity without corrupting explicit relation bindings. ### Check List (For Author) - Test: Unit Test - 43 FE tests passed across DropTableStreamTest, StatementContextTest, ExplainTableStreamPlanTest, and MTMVRelationTest - StatementContextTest rerun: 11 tests passed - Java compilation and Checkstyle passed - Behavior changed: Yes. Stream base dependencies use a stable per-statement object snapshot for planning locks and MTMV dependency tracking. - Does this need documentation: No
|
run buildall |
|
/review |
There was a problem hiding this comment.
Requesting changes for two correctness gaps that remain on 25d0c27f52c20efa8496732fcb0ab457e21e9377:
- P1 — persisted MTMV compatibility: newly analyzed creation/refresh now tracks the stable stream base, but load compatibility does not repair an already persisted malformed relation. The base can remain absent from commit routing and freshness snapshots, allowing stale rewrite results after upgrade.
- P2 — database recovery lifecycle: the cached-base check observes only
Table.isDropped.RECOVER DATABASEclears that flag before the owning database is published, so a cross-database stream can expose the detached base during recovery and potentially after a failed publication attempt.
Critical-checkpoint conclusions:
- Goal and proof: the patch correctly makes direct table drop/recover affect stream state and repairs stable dependency capture for current planning. The unit/regression additions cover direct table lifecycle, identity capture, locking/preload reuse, and current MTMV relation construction, but they do not prove cross-database recovery or old-image migration; those are the two blocking gaps above.
- Focus: production changes are otherwise focused on stream availability, stable planner dependencies, and their metadata consumers. No extra user focus was supplied.
- Concurrency: collection, preload, and locking now reuse one identity-stable dependency snapshot and table locks remain ID-ordered. The remaining race is the database-recovery publication ordering described inline; no additional lock-order cycle was found.
- Lifecycle: cached stream bases, recycle state, drop/recover-as, force-drop/replacement, replay/load, and erase paths were traced. The table-only lifecycle fence is incomplete for an unpublished owning database.
- Configuration: no production configuration or dynamic-update contract is added. Test-only flags are isolated by FE Surefire's per-class JVM behavior.
- Compatibility: there is no FE/BE protocol, symbol, or storage-format change. Catalog rolling-upgrade compatibility is incomplete for an existing MTMV relation that omits the stable stream base.
- Parallel paths and conditions: both
RECOVER TABLEandRECOVER DATABASE, cache-hit/cache-miss resolution, creation/refresh, nested views, preload/locking, invalidation, freshness, and rewrite discovery were checked. The changed dropped-table condition fixes the reported table-recovery path but misses database recovery. - Tests and results: the changed tests and ordered
.outexpectations were inspected for fidelity and determinism. No builds or tests were run because the authoritative review task prohibited execution. Missing async-MTMV coverage already requested in an existing thread was not duplicated. - Observability: existing identifiers and log levels are sufficient for the new normal paths; no separate metrics issue was found. The recycle lookup's downgrade to DEBUG did not hide a distinct actionable failure.
- Persistence and transactions: no data-write transaction path changes. MTMV relation/snapshot persistence and registration were reviewed; the old-relation migration gap is the persistence finding above. Edit-log/replay changes otherwise remain equivalent.
- Data writes and FE/BE variables: no new data-write atomicity surface or FE-to-BE variable is introduced.
- Performance: identity deduplication avoids double locking, and no material new CPU, memory, or redundant-work issue was found.
- Other issues: all 14 changed files, direct consumers, and existing review threads were rechecked. A second full-review round with a separate risk pass converged with no additional valuable findings.
| baseTable = baseTableInfo.getTableNullable(); | ||
| TableIf cachedBaseTable = baseTable; | ||
| if (cachedBaseTable != null) { | ||
| if (cachedBaseTable instanceof Table && ((Table) cachedBaseTable).isDropped) { |
There was a problem hiding this comment.
[P2] Keep cached bases unavailable until their database recovers
A stream can live in ds while its base A lives in db. During RECOVER DATABASE db, recoverAllTables() calls Database.registerTable(A) and clears A.isDropped before InternalCatalog.recoverDatabase() republishes and unmarks db. If the stream retained its pre-drop cached pointer (because it was not read while db was dropped), this branch returns A in that gap, so metadata and planning expose it while its owning database is unavailable; if the later catalog lock/name check fails, that exposure can outlive the recovery attempt because the recycle entries were already removed. This is distinct from the existing RECOVER TABLE cache-miss thread because here the table flag already passes. Please fence on the owning database lifecycle as well (or defer clearing member-table dropped flags), and cover cross-database recovery with a latch test.
| return Pair.of(Sets.newHashSet(ctx.getStatementContext().getTables().values()), | ||
| Set<TableIf> baseTables = Sets.newHashSet(ctx.getStatementContext().getTables().values()); | ||
| // Implicit dependencies are all-level tables, not relations written at the first query level. | ||
| baseTables.addAll(ctx.getStatementContext().getImplicitTableDependencies()); |
There was a problem hiding this comment.
[P1] Migrate persisted stream dependencies on upgrade
These additions repair newly analyzed creation and refresh, but an MTMV image can already contain stream S without stable base T: old dependency collection could name-resolve replacement B while the stream wrapper still scanned its cached T, then persist a successful refresh as {S, B} with no T snapshot. On load, MTMVRelation.compatible() only normalizes stored entries and MTMV.compatible() re-registers them unchanged. T therefore has no commit-event edge or recorded snapshot, while freshness treats S as synchronous; the MTMV can remain rewrite-eligible with stale rows until a later refresh succeeds. Please expand persisted stream dependencies before registration (or conservatively invalidate them) and add an old-image upgrade test.
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: close apache#65418 Related PR: apache#66287 Problem Summary: A cached stream base table could become visible while its dropped database was only partially recovered. Existing MTMV images could also omit the stream's stable base dependency, leaving no invalidation edge or freshness snapshot after upgrade. Require the owning internal database and its stable table mapping to be published before exposing a stream base, and migrate persisted MTMV relations to include stable stream bases without fabricating historical snapshots. ### Release note Streams remain unavailable until their owning database is fully recovered, and existing MTMVs migrate stable stream base dependencies during upgrade. ### Check List (For Author) - Test: Unit Test - DropTableStreamTest and MTMVRelationTest: 12 tests passed - Java compilation and Checkstyle passed - Behavior changed: Yes. Stream availability now follows database recovery lifecycle, and old MTMV relations gain stable stream base dependencies. - Does this need documentation: No
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Review context preparation failed before Codex ran; inspect the 'Prepare authoritative PR context and required AGENTS guides' step. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
TPC-H: Total hot run time: 29222 ms |
TPC-DS: Total hot run time: 158645 ms |
ClickBench: Total hot run time: 23.96 s |
|
/review |
There was a problem hiding this comment.
Request changes: one P1 persisted-compatibility gap remains.
The ordinary stable-ID drop/recovery flow is substantially improved: stream availability now rejects dropped tables and unpublished databases, metadata derives one coherent availability snapshot, planner collection keeps implicit bases out of the qualifier cache while reusing the captured object for preload and locking, and newly analyzed/refreshed MTMVs record the underlying base. The remaining blocker is the old-image migration path when the persisted stream itself is temporarily in the recycle bin: compatibility silently skips expansion, so same-name stream recovery can later restore rewrite eligibility without the base snapshot or invalidation edge.
Critical checkpoint conclusions:
- Goal and proof: the changed unit and regression tests cover ordinary recoverable/force table drop, same-ID recovery and rename, database recovery publication, planner lock/preload reuse, relation-set construction, and available-stream compatibility. They do not cover the blocking old-image drop/recover-stream sequence described inline.
- Scope and clarity: the production changes are focused on stable-ID availability, display metadata, planner dependencies, and MTMV compatibility. No unrelated source changes were found.
- Concurrency and locking: table/database dropped flags, live catalog identity, and exact table identity fail closed during recovery publication. The captured implicit dependency participates in the existing ID-ordered planner lock queue, and direct relations, views, CTEs, and subqueries all collect before external preload. No additional lock-order, remote-I/O-under-lock, or unlocked-replacement defect was substantiated.
- Lifecycle, persistence, and compatibility: this is the blocking checkpoint. An unresolved stream relation is treated differently from an unresolved stream base: the former is silently skipped, while the latter fails compatibility and marks the MTMV schema-changed. Generic stream recovery does not rerun relation expansion, leaving the historical relation and snapshot incomplete.
- Parallel paths and conditions: direct stream planning, view-contained streams, insert targets, external metadata preload, MTMV creation, refresh, replay registration, missing snapshots, and null/empty legacy relation-set fallbacks were checked. No second non-duplicate issue remained.
- Configuration, protocol, and storage compatibility: no configuration item, FE/BE variable, RPC/thrift field, function symbol, or storage-format change is introduced.
- Transactions and data correctness: no data-write path changes directly, but the accepted compatibility gap can admit a stale MTMV for rewrite after writes to its untracked base, so it is correctness-blocking.
- Performance and observability: the added identity sets, set copies, and catalog lookups are bounded by statement/relation metadata. Reducing expected unavailable-base lookup logging from warning to debug is appropriate; no additional metric or logging gap was found.
- Tests: tests were inspected but not executed because this review environment explicitly prohibits builds and test runs. The added regression uses ordered output and the FE concurrency tests use latches rather than timing sleeps.
- User focus: no additional review focus was provided; the full PR was reviewed.
Existing review threads were re-read immediately before submission. Substantially similar earlier concerns were suppressed rather than repeated.
| TableIf table; | ||
| try { | ||
| table = MTMVUtil.getTable(info); | ||
| } catch (AnalysisException e) { |
There was a problem hiding this comment.
[P1] Do not complete migration while the stream is missing
For an old image where M records stream S but not its base T, recoverably drop S before loading the image. MTMVUtil.getTable(info) throws here, but this continue lets compatibility succeed and re-register only the old S relation. After same-name RECOVER TABLE S, compatibility is not rerun; freshness treats the recovered stream as synchronous, while M still has neither a snapshot nor an invalidation edge for T, so writes to T can leave stale rows rewrite-eligible. This is a residual branch in the newly added migration, distinct from the prior available-stream case. Please fail compatibility or persist a pending migration when the relation cannot be resolved, or rerun expansion before a recovered stream can make the MTMV a candidate, and add an old-image drop/recover-stream test.
4 participants
What problem does this PR solve?
Issue Number: #64518
Related PR: #61382
Problem Summary:
Dropping a stream base table left the stream enabled and non-stale in
information_schema.table_streams. Recoverable drops could also keep usingthe cached dropped table instead of resolving the persisted base table
identity.
Root cause:
BaseTableStream.getBaseTableNullable()retained a cachedTableafter it was marked dropped, whileisDisabled(),isStale(), andgetStaleReason()only returned their persisted flags.Change Summary:
BaseTableStream.javaTableStreamManager.javaN/A.TableStreamBaseTableInfo.javaDropTableStreamTest.javastateDiagram-v2 [*] --> Available Available --> Stale: drop base table Stale --> Available: recover original table ID Stale --> Stale: create same-name table with new IDRelease note
Streams whose base tables are unavailable are now reported disabled and
stale.
Check List (For Author)
Test
Local verification:
./run-fe-ut.sh --run org.apache.doris.catalog.DropTableStreamTest(4 tests)env DISABLE_BUILD_UI=ON ./build.sh --feBehavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)